DOC Update coding conventions by emteknetnz · Pull Request #397 · silverstripe/developer-docs

emteknetnz · 2023-11-06T05:42:13Z

Issue #323

~~Right now it requires some discussion with @silverstripe/core-team to see everyone's happy with the conventions that I've added in~~ < This has been done

This PR will only focus on PHP. I've spun off a separate card for JS + SCSS

michalkleiner · 2023-11-06T06:51:07Z

👍 Generally agree with all the proposed additions. It'd be great to have linter/phpcs/PhpStan rules for as many of these as possible.

GuySartorelli

There's some stuff here that seems like a duplication of the stuff below your additions - what's the intention there? Will you be removing all the old standards? Will you do a de-dupe separately? Or are the duplications not intended?

I'll hold off reviewing the existing standards until you've answered this.

I also assume that the other language coding standards have yet to be done.

ScopeyNZ

Mostly feel strongly about trying to ratify member visibility, as some may remember from silverstripe/silverstripe-framework#8996.

maxime-rainville

There's a lot of good stuff in there!!!

I would make the following assumptions explicit once and avoid repeating them over and over:

Best practices are approaches that will work well in most situations.
It's OK to occasionally ignore them. (Maybe we want to have two classes ... the ones that are nice to have but not so important ... and the ones that should only be ignored as a last results)
Best practices have not always been systematically applied, so you will encounter instances where they are not followed.

In term of language, I would be inclined to try to normalise it and avoid complicated dissertation. Maybe remove in-depth explanation of why we think something is a good.

I would focus on using the words "avoid" or "prefer" in most places. Maybe avoid using expression like "where possible" or "if practical".

Some extra questions we might want to address:

Do we prefer Promises or Async/Await in JS?
Prefer using the ORM instead of raw SQL.
Prefer DataObjectSchema::tableName() to get the table name for DataObject rather then hardcode the value.
Prefer generic/standard SQL syntax that work across all DataBase.

ScopeyNZ · 2023-11-10T03:00:42Z

Do we prefer Promises or Async/Await in JS?

I'm against mentioning a preference here. They have different use-cases. Both are situational, and may even be used together, eg:

const thing = await fetch('url').then(response => response.json());

Prefer DataObjectSchema::tableName() to get the table name for DataObject rather then hardcode the value.

This should be a rule (at least it was when I was last peer-reviewing 😅 )

emteknetnz · 2023-11-13T03:34:24Z

Have made recommended edits. Have also updated the content that was previously on the page and either made it much more concise or removed. Ready for review again

GuySartorelli

@maxime-rainville in #397 (review) you said

Maybe avoid using expression like "where possible" or "if practical".

There's still some of that in here so you may want to look and see if you're okay with the way that language has been used in the new revision.

GuySartorelli

Still not done:

GuySartorelli · 2023-11-14T02:31:58Z

@maxime-rainville here are the specific things you need to double check:

maxime-rainville

Maybe avoid using expression like "where possible" or "if practical".

There's still some of that in here so you may want to look and see if you're okay with the way that language has been used in the new revision.

I had a glance and didn't see anything obviously standing out. By general point was that "where possible" or "if practical" is kind of implied when drafting best practices.

GuySartorelli

LGTM. No outstanding requested changes.

emteknetnz force-pushed the pulls/5/code-standards branch from dd0e3a0 to fe5ef64 Compare November 6, 2023 05:47

emteknetnz mentioned this pull request Nov 6, 2023

Review best practice docs #323

Closed

wilr reviewed Nov 6, 2023

View reviewed changes

Comment thread en/05_Contributing/05_Coding_Conventions/01_PHP_Coding_Conventions.md Outdated

michalkleiner reviewed Nov 6, 2023

View reviewed changes